Skip to content

fix: use supported wkhtmltox packages for Linux setup#4277

Open
zay168 wants to merge 2 commits intofrappe:developfrom
zay168:codex/fix-4197-wkhtmltox-install
Open

fix: use supported wkhtmltox packages for Linux setup#4277
zay168 wants to merge 2 commits intofrappe:developfrom
zay168:codex/fix-4197-wkhtmltox-install

Conversation

@zay168
Copy link
Copy Markdown

@zay168 zay168 commented Mar 27, 2026

closes #4197

Summary

  • switch the Linux helper to the official wkhtmltopdf packaging releases for Debian and Ubuntu derivatives
  • use the maintained bookworm and jammy packages that depend on libssl3
  • update the local setup note in the README to point contributors to the current installation doc and the distro-specific wkhtmltox packages

Why

The current installation path around wkhtmltox_file.deb is misleading for modern Debian and Ubuntu derivatives. The maintained packaging release assets provide the correct filename pattern and avoid the legacy libssl1.1 dependency that blocks manual installs on newer systems.

Validation

  • checked the referenced jammy and bookworm assets exist in wkhtmltopdf/packaging release 0.12.6.1-3
  • inspected both .deb control files to confirm they depend on libssl3 and not libssl1.1
  • ran bash -n .github/helper/install.sh with Git Bash
  • ran git diff --check

Summary by CodeRabbit

  • New Features

    • Simplified installation for Debian and Ubuntu systems with automatic selection of compatible wkhtmltox packages.
  • Documentation

    • Updated framework documentation links.
    • Added platform-specific installation guidance for Debian and Ubuntu.

@zay168 zay168 requested a review from asmitahase as a code owner March 27, 2026 19:23
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

Walkthrough

The change modifies the wkhtmltopdf installation process and documentation. The install script now intelligently selects and installs distribution-specific prebuilt wkhtmltox packages based on OS identifiers and CPU architecture from GitHub releases, with fallback to the generic tar.xz method if no matching package exists. The README is updated to reference the current Frappe documentation URL and includes instructions for installing the appropriate distro-specific wkhtmltox package for Debian/Ubuntu systems.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adopting official wkhtmltox packages for Linux setup instead of legacy solutions.
Linked Issues check ✅ Passed The PR successfully addresses both objectives from issue #4197: uses modern libssl3-dependent packages (bookworm/jammy) and updates installation documentation with current distro-specific instructions.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the Linux wkhtmltox installation issue; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/helper/install.sh:
- Around line 47-50: The current early return when installing the .deb for
wkhtml_package gets bypassed by set -e because failures abort the script; change
the logic around the wkhtml_package block so the wget + apt install attempt is
tried but does not exit the script on failure—capture the commands' exit status
and only return when they succeed (e.g., run wget and sudo apt install for the
variable wkhtml_package, check their exit codes, and if both succeed then
return; otherwise continue to the tarball fallback). Ensure you reference the
wkhtml_package variable and the block that currently does wget -O
"/tmp/${wkhtml_package}" ... and sudo apt install ... "/tmp/${wkhtml_package}"
and replace the unconditional return with a conditional return based on success.

In `@README.md`:
- Around line 96-98: The fenced shell code block contains a leading shell prompt
character ('$') before the command 'bench start' which violates markdownlint
rule MD014; remove the leading '$' so the block contains just the raw command
(i.e., replace "$ bench start" with "bench start" in the fenced code block) to
satisfy the linter while keeping the command example intact.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d8fbdd8b-3b06-4d98-a8e6-c47de9cc63d6

📥 Commits

Reviewing files that changed from the base of the PR and between 7a63b5e and 03c3d23.

📒 Files selected for processing (2)
  • .github/helper/install.sh
  • README.md

Comment on lines 96 to 98
```sh
$ bench start
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove shell prompts in command examples to satisfy markdownlint.

At Line [97], drop the leading $ in the fenced shell block (MD014) to keep docs lint-clean.

🧰 Tools
🪛 markdownlint-cli2 (0.22.0)

[warning] 97-97: Dollar signs used before commands without showing output

(MD014, commands-show-output)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 96 - 98, The fenced shell code block contains a
leading shell prompt character ('$') before the command 'bench start' which
violates markdownlint rule MD014; remove the leading '$' so the block contains
just the raw command (i.e., replace "$ bench start" with "bench start" in the
fenced code block) to satisfy the linter while keeping the command example
intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to reproduce steps for Linux Install provided in the guide

1 participant